Skip to content

[PM-5756] Comment on originating PR with BIT results #373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 13, 2025
Merged

[PM-5756] Comment on originating PR with BIT results #373

merged 9 commits into from
Aug 13, 2025

Conversation

jprusik
Copy link
Contributor

@jprusik jprusik commented Aug 8, 2025

🎟️ Tracking

PM-5756

📔 Objective

This adds repository dispatch triggers (on "trigger-bit-tests" event) to the Test-all and Test-all-custom-flags workflows. The workflows will now additionally comment with the BIT results on the originating PR.

(The ENABLE_PR_FEEDBACK var can be updated to a value other than "true" (or even deleted) to turn off commenting on the origin PR. )

The corresponding clients work: bitwarden/clients#15960

Testing

Permissioning and commenting tested/verified in #375; receipt verified in bitwarden/clients#15988

📸 Screenshots

Test-all success
Screenshot 2025-08-12 at 2 45 54 PM
Test-all failure
Screenshot 2025-08-12 at 2 45 47 PM
Test-all-custom-flags failure
Screenshot 2025-08-12 at 2 45 26 PM
Test-all-custom-flags success
This case does not send a report back to the originating PR

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@jprusik jprusik changed the title Comment on originating PR with BIT results [PM-5756] Comment on originating PR with BIT results Aug 8, 2025
@jprusik jprusik self-assigned this Aug 8, 2025
Copy link

github-actions bot commented Aug 8, 2025

Logo
Checkmarx One – Scan Summary & Details05922665-6c8f-4da8-bba2-4d00bc93878c

Great job! No new security vulnerabilities introduced in this pull request

Comment on lines +152 to +180
# Only post back to the PR if there was a failure since test-all has the
# typical config case covered for success feedback
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasoning here being, we don't need to be overly noisy if everything looks good, but we still want to alert if these cases fail.

@jprusik jprusik requested a review from differsthecat August 12, 2025 17:13
@jprusik jprusik marked this pull request as draft August 12, 2025 17:13

- name: Communicate BIT failure on originating issue
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
if: failure() && github.event.client_payload.origin_issue
Copy link
Contributor Author

@jprusik jprusik Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally did not use step failure (of, for example, the playwright-tests step), so that we still report back if BIT failed somewhere else. If we find this creates too much noise (like if the test login process starts to fail from a ui update), we can target the step failure (by id) instead.

If the action is cancelled, neither case fires.

@jprusik jprusik force-pushed the pm-5756 branch 4 times, most recently from 27c6be4 to ee24ed7 Compare August 12, 2025 21:26
@jprusik jprusik marked this pull request as ready for review August 12, 2025 21:26
Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with the azure steps if we run this manually? Do the Azure login steps also fire? Do we want to scope those to only run when this is triggered from a dispatch?

@jprusik
Copy link
Contributor Author

jprusik commented Aug 13, 2025

What happens with the azure steps if we run this manually? Do the Azure login steps also fire? Do we want to scope those to only run when this is triggered from a dispatch?

Good call; yeah they would (they fire again when building the dotenv in in "Create dotenv file" step as well). I'll give it another look... 👍

Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you @jprusik !

@jprusik jprusik merged commit b4f909e into main Aug 13, 2025
8 checks passed
@jprusik jprusik deleted the pm-5756 branch August 13, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants